[PWGHF] Replace UPC Lc THnSparse with TTree#15786
[PWGHF] Replace UPC Lc THnSparse with TTree#15786Rrantu wants to merge 7 commits intoAliceO2Group:masterfrom
Conversation
Rrantu
requested review from
NicoleBastid,
alibuild,
apalasciano,
deepathoms,
fcatalan92,
fcolamar,
fgrosa,
gluparel,
hahassan7,
jpxrk,
mfaggin,
mmazzilli,
singhra1994,
stefanopolitano,
vkucera and
zhangbiao-phy
as code owners
github-actions
bot
changed the title
|
O2 linter results: ❌ 0 errors, |
|
@Rrantu Columns and tables are parts of the data model. They don't belong in the utility headers. |
|
Hi @vkucera, thanks for the comment, where would you recommend moving the columns and tables to? Would it be acceptable to define them directly in the task? |
Yes, if the tables are not used anywhere else, please put them in the task. |
Thanks! I‘ve moved them to the task file |
| DECLARE_SOA_TABLE(HfUpcLcInfos, "AOD", "HFUPCLCINFOS", | ||
| full::M, | ||
| full::Pt, | ||
| full::PtProng0, |
There was a problem hiding this comment.
Why do you duplicate columns in both tables?
There was a problem hiding this comment.
One table corresponds to the BDT-applied version, while the other does not. However, both require the mass and pt information.
There was a problem hiding this comment.
You never fill both tables. It would be better to have one table with the common columns that you fill in any case and a second table with the extra columns for the BDT case.
| DECLARE_SOA_COLUMN(ZdcTimeZNC, zdcTimeZNC, float); | ||
| } // namespace full | ||
|
|
||
| DECLARE_SOA_TABLE(HfUpcLcBdtInfos, "AOD", "HFUPCLCBDTINFOS", |
There was a problem hiding this comment.
Why do you call this table BDT?
There was a problem hiding this comment.
Because the tree includes BDT scores for the candidates.
There was a problem hiding this comment.
But it also includes other BDT-unrelated columns. See my comment above.
There was a problem hiding this comment.
Thanks for the suggestion. Here the detector variables and the BDT output are both required together for offline selections. Given this use case, I would prefer to keep the current structure.
| // ThnSparse for ML outputScores and Vars | ||
| Configurable<bool> fillTHn{"fillTHn", false, "fill THn"}; | ||
| Configurable<bool> fillUPCTHnLite{"fillUPCTHnLite", false, "fill THn"}; | ||
| Configurable<bool> fillUPCTreeLite{"fillUPCTreeLite", false, "fill THn"}; |
There was a problem hiding this comment.
You only have a single option to fill the trees. It seems that the attributes UPC and Lite are redundant.
There was a problem hiding this comment.
And the comment is wrong.
There was a problem hiding this comment.
Thanks for the comment! The Lite one fills fewer candidates, including only those passing the single-gap selection.
There was a problem hiding this comment.
Then it should be called something like fillTreeOnlySingleGap and explained in the comment.
There was a problem hiding this comment.
Thanks for the suggestion, I have renamed it.
|
@Rrantu A conceptual question: Why can you not use the THnSparse? |
Thanks for the question! For large statistics, merging THnSparse objects becomes very heavy, making it difficult to obtain merged AnalysisResults.root. I also need to repeatedly adjust selections on multiple axes during the offline analysis. Projections from high-dimensional THnSparse are not very flexible. |
|
Dear @Rrantu, @vkucera, all codeowners, sorry for interfering, |
No, they are not. |
|
Hi @lubynets, Thanks for the question. In this case, the tables are only used within this task, and the trees are produced specifically for this analysis. Also, there are other tasks that produce trees in a similar way. |
3 participants
This PR introduces a transition from THnSparse to TTree in the UPC Lc analysis.
Main changes: